Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ProperEscapingFunction: fix overreach #668

Merged
merged 1 commit into from
Apr 21, 2021
Merged

ProperEscapingFunction: fix overreach #668

merged 1 commit into from
Apr 21, 2021

Conversation

jrfnl
Copy link
Collaborator

@jrfnl jrfnl commented Apr 20, 2021 •
edited
Loading

As part of the changes made in #624, the T_COMMA token was added to the list of tokens to skip over, to allow for echo statements with multiple arguments passed as a coma-delimited list.

As a side-effect, this caused the sniff to also examine [s]printf()-like function calls where the first parameter is a text string, while the second is often a variable within a call to one of the escaping functions.

The current change fixes this by only adding the T_COMMA token to the "ignore when looking for the previous token"-list when in an echo statement.

Includes unit test.

Fixes #667

Additional notes:

  • I've run the sniff over WP Core to verify the fix and have verified that all 23 violations being throw up are correctly detected violations.
  • If it would be considered a good idea to also examine, [s]printf()-like function calls for this sniff for proper escaping, I suggest opening a separate, new feature request as that change would need significantly different and quite complex logic and does not fall within the scope of this bug fix.

@jrfnl
ProperEscapingFunction: fix overreach
cc1022e 
As part of the changes made in 624, the `T_COMMA` token was added to the list of tokens to skip over to allow for `echo` statements with multiple arguments passed as a coma-delimited list.

As a side-effect, this caused the sniff to also examine `[s]printf()`-like function calls where the first parameter is a text string, while the second is often a variable within a call to one of the escaping functions.

The current change fixes this by only adding the `T_COMMA` token to the "ignore when looking for the previous token"-list when in an `echo` statement.

Includes unit test.

Fixes 667

Additional notes:
* I've run the sniff over WP Core to verify the fix and have verified that all 23 violations being throw up are correctly detected violations.
* If it would be considered a good idea to also examine, `[s]printf()`-like function calls for this sniff for proper escaping, I suggest opening a separate, new feature request as that change would need significantly different and quite complex logic and does not fall within the scope of this bug fix.
@jrfnl jrfnl added this to the 2.3.1 milestone Apr 20, 2021
@jrfnl jrfnl requested a review from a team as a code owner April 20, 2021 13:28
@jrfnl jrfnl mentioned this pull request Apr 20, 2021
Closed
2 tasks
@westonruter
Copy link

It works much better but there are still false positives involving esc_attr_x():

FILE: includes/validation/class-amp-validated-url-post-type.php
----------------------------------------------------------------------
FOUND 5 ERRORS AFFECTING 5 LINES
----------------------------------------------------------------------
 2370 | ERROR | Wrong escaping function, using `esc_attr_x()` in a
      |       | context outside of HTML attributes may not escape
      |       | properly.
      |       | (WordPressVIPMinimum.Security.ProperEscapingFunction.notAttrEscAttr)
 2379 | ERROR | Wrong escaping function, using `esc_attr_x()` in a
      |       | context outside of HTML attributes may not escape
      |       | properly.
      |       | (WordPressVIPMinimum.Security.ProperEscapingFunction.notAttrEscAttr)
 2425 | ERROR | Wrong escaping function, using `esc_attr_x()` in a
      |       | context outside of HTML attributes may not escape
      |       | properly.
      |       | (WordPressVIPMinimum.Security.ProperEscapingFunction.notAttrEscAttr)
 2456 | ERROR | Wrong escaping function, using `esc_attr_x()` in a
      |       | context outside of HTML attributes may not escape
      |       | properly.
      |       | (WordPressVIPMinimum.Security.ProperEscapingFunction.notAttrEscAttr)
 2461 | ERROR | Wrong escaping function, using `esc_attr_x()` in a
      |       | context outside of HTML attributes may not escape
      |       | properly.
      |       | (WordPressVIPMinimum.Security.ProperEscapingFunction.notAttrEscAttr)
----------------------------------------------------------------------

See annotations in PR.

@westonruter
Copy link

westonruter commented Apr 20, 2021 •
edited
Loading

Sorry, I was mistaken. Our code was using esc_attr_x() when we should have been using esc_html_x(), so the sniff caught a bug! True positive!

@jrfnl
Copy link
Collaborator Author

jrfnl commented Apr 20, 2021

Thanks for testing @westonruter and glad to hear that this PR fixes the issue.

rebeccahum
rebeccahum approved these changes Apr 21, 2021
View reviewed changes
Copy link
Contributor

@rebeccahum rebeccahum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested and works great!

@rebeccahum rebeccahum merged commit 7decb5b into develop Apr 21, 2021
@rebeccahum rebeccahum deleted the fix/667-properescaping-overreach branch April 21, 2021 00:22
@manooweb
Copy link

Hello,
When this fix will be released please?

@rebeccahum rebeccahum mentioned this pull request Apr 21, 2021
Closed
@rebeccahum
Copy link
Contributor

@manooweb Hi! This will be released with 2.3.1, which we are hoping to within the next few days.

@manooweb
Copy link

Ok 👍 thanks

@spacedmonkey spacedmonkey mentioned this pull request Apr 27, 2021
Merged
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rebeccahum rebeccahum rebeccahum approved these changes

Assignees
No one assigned
Labels
Type: Bug
Projects
None yet
Milestone
2.3.1
Development

Successfully merging this pull request may close these issues.

False positive for notAttrEscAttr in ProperEscapingFunction sniff when printf()/sprintf() used
4 participants
@jrfnl @westonruter @manooweb @rebeccahum